Skip to content

Log-scale layout of heatmap bricks for log-scale axes (fixes #5991) - #6819

Merged
archmoj merged 11 commits into
plotly:masterfrom
andrew-matteson:log-scale-heatmap
Jan 26, 2024
Merged

Log-scale layout of heatmap bricks for log-scale axes (fixes #5991)#6819
archmoj merged 11 commits into
plotly:masterfrom
andrew-matteson:log-scale-heatmap

Conversation

@andrew-matteson

Copy link
Copy Markdown
Contributor

This fixes the heatmap brick layout identified in #5991 . The logic of make_bounds_array is extended with logic that replicates the linear scale logic in log space.

This PR fixes two problems in my use of the library:

  1. Log-scale heatmaps would not render blocks that calculate a bound < 0.
  2. Log-scale heatmaps would not have evenly sized blocks.

I've added a mock and baseline that behave correctly.

Comment thread test/image/mocks/heatmap-log-scale.json
@andrew-matteson

Copy link
Copy Markdown
Contributor Author

Bumping this @archmoj after the winter holiday. I'm hoping to get this into a near-term release for my team.

@andrew-matteson

Copy link
Copy Markdown
Contributor Author

@archmoj Can you let me know anything that needs to happen to get this merged?

@archmoj archmoj added bug something broken community community contribution status: reviewable labels Jan 17, 2024
@archmoj

archmoj commented Jan 17, 2024

Copy link
Copy Markdown
Contributor

Thanks for the fix. 🥇
Looking very good to me.
Over to @alexcjohnson

@archmoj
archmoj requested a review from alexcjohnson January 17, 2024 20:32
Comment thread src/traces/heatmap/make_bound_array.js Outdated
@alexcjohnson

Copy link
Copy Markdown
Collaborator

@archmoj after my two comments above this looks good. Thanks @andrew-matteson!

There's still an issue with histogram2d failing on log axes if the bins go past zero, but that's a trickier problem, we need to decide if the bins should also be spaced geometrically (and if so support binning that way) but that feels more like a new feature than a fix like the rest of this PR, since it will alter the data to be displayed, not just the details of how it's displayed - so it doesn't belong here - and is probably a much lower priority anyway, I don't think histogram2d gets a whole lot of use.

@andrew-matteson

Copy link
Copy Markdown
Contributor Author

@alexcjohnson I think my fixes cover your cases. LMK if you have further thoughts.

@andrew-matteson

Copy link
Copy Markdown
Contributor Author

There is a webgl-jasmine failure that I don't think is related to the recent changes. If it is, could you let me know what's going on? I can't quite deduce from the error messages.

@andrew-matteson

Copy link
Copy Markdown
Contributor Author

@alexcjohnson I got the tests to rerun and they passed. LMK if this needs anything else.

Comment thread CHANGELOG.md Outdated
@andrew-matteson

Copy link
Copy Markdown
Contributor Author

@archmoj @alexcjohnson I'd love to get this in for an upcoming release my team has. Is there anything I need to do to wrap this up?

@archmoj

archmoj commented Jan 26, 2024

Copy link
Copy Markdown
Contributor

Thanks very much for the fix.
💃
We are planning to have a minor plotly.js release next week which would include this.

@archmoj
archmoj merged commit 259abab into plotly:master Jan 26, 2024
@andrew-matteson

Copy link
Copy Markdown
Contributor Author

Thank you! I appreciate this product so much :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug something broken community community contribution

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants